fix: harden security beyond PR #249 — command injection, deps, path injection - #264
fix: harden security beyond PR #249 — command injection, deps, path injection#264WODE25500 wants to merge 5 commits into
Conversation
…s, path injection
Five additional security hardening changes identified during a full
repository security audit:
1. Replace os.system() with subprocess.run() in Sleep plugin
(plugins/openclaw/slash_sleep.py) to prevent shell command injection
via unsanitized arguments.
2. Raise dependency floors to address known CVEs:
- vllm >= 0.8.4 (was 0.4.0; CVE-2025-32433 in transitive deps)
- datasets >= 3.0 (was 2.18.0; remote code execution via
load_dataset with untrusted configs)
- Declare openai-codex-sdk as an explicit optional dep (codex extra)
to prevent dependency confusion / undeclared-import attacks.
3. Sanitize task_id before use in tempfile.mkdtemp prefix
(skillopt/envs/spreadsheetbench/rollout.py) to prevent directory
creation at attacker-chosen paths via crafted task identifiers.
4. Extend WebUI security tests from 2 to 8, covering --share warning,
auth via CLI args / env vars, default-no-auth, and path traversal
rejection in scan_outputs().
5. Sync requirements.txt commented versions with pyproject.toml floors.
All 1445 existing tests pass; 6 new regression tests added.
|
The shell-free subprocess invocation and path hardening are useful. Re-reviewing In Both call Please reject incomplete credentials before building/launching the UI. Tests should cover user-only, password-only, incomplete environment configuration, and a complete pair, with the incomplete cases asserting that |
Supplying only --auth-user or only --auth-pass (or only one of SKILLOPT_WEBUI_USER / SKILLOPT_WEBUI_PASS) previously left auth=None and still launched the UI — a deployment could expose the training controls without login. Now reject before building/launching (sys.exit 1); launch() is never called for incomplete credentials. Added user-only / pass-only / env-incomplete regressions.
|
Yifan Yang (@Yif-Yang) — fixed on |
|
Thanks for I am keeping the security-hardening review open rather than treating those passing cases as proof that the complete WebUI boundary is covered. Please extend the negative integration matrix through the real Any further security-sensitive reproduction details should be coordinated privately under the repository's |
|
Understood, and agreed. Any further security-sensitive reproduction details (real credentials, private filesystem paths, or exploit payloads) will be coordinated privately per |
…oint Lift scan_outputs out of the build_ui closure so the Output Explorer callback is directly testable, and add callback-level tests that call it with traversal args (denied, returns []) and a valid in-tree output area (digested, reads config.yaml). This replaces the prior approximation tests that only re-checked relative_to() in isolation.
|
Thanks for the re-review. I reworked the Output Explorer tests so they exercise the actual data-consumption path instead of re-checking the containment helper in isolation.
Full webui suite: 21 passed. (If desired I can extend the same callback-level approach to the remaining UI callbacks.) |
The codex optional extra and the vllm/datasets floor bumps are dependency hygiene / CVE-floor changes, not part of the command-injection and path- injection hardening. Keep this PR surgical (the four security fixes + WebUI tests); the dependency changes are preserved in the branch history (commit 6f0030d) for a separate dependency PR. The gradio floor comment stays as-is (already synced to pyproject's 5.50.0 floor).
UI callbacks must not trust Gradio component values: config preview and launch preflight now resolve paths through a shared boundary helper and only accept configs/ files, while scan_outputs also rejects symlink escapes at every directory/file it reads. Config preview was promoted to a module-level callback so the registered consumption path is directly testable.
|
Yifan Yang (@Yif-Yang) Re-reviewing the complete WebUI boundary, I found the config-preview callback ( Fix on
Added callback-level regressions for relative traversal, absolute traversal, a valid in-tree config, and launch-preflight rejection; the full WebUI test set (17 security + build/env preflight) passes locally: 25 passed. CI on the new head is awaiting maintainer approval. |
2 participants
Summary
Post-#249 security hardening from a full-repo audit. Kept surgical: the dependency/CVE floor changes and the
codexoptional extra that were originally bundled here are split OUT of this PR so it contains only the security fixes.Changes
Command injection fix - Replace
os.system()withsubprocess.run()in the Sleep plugin (plugins/openclaw/slash_sleep.py). The command is passed as a list with no shell, so unsanitized arguments can no longer be injected.Path injection fix - Sanitize
task_idbefore use intempfile.mkdtempprefix (skillopt/envs/spreadsheetbench/rollout.py) to prevent directory creation at attacker-chosen paths.WebUI hardening:
scan_outputsguarded so it digests only underPROJECT_ROOT(path-traversal and symlink escapes are denied at the point each directory/file is read).PROJECT_ROOT/configs/(load_config,validate_training_config).--auth-user/--auth-pass(orSKILLOPT_WEBUI_USER/SKILLOPT_WEBUI_PASS) basic auth, failing closed on incomplete credentials; a warning when--shareis used.Test plan
python -m pytest tests/test_webui_security.py(17 passed)python -m pytest tests/test_webui_security.py tests/test_webui_build_gradio.py tests/test_webui_env_preflight.py(25 passed)action_required)Note for maintainers
The dependency changes originally here (
codexoptional extra,vllm>=0.8.4,datasets>=3.0) are intentionally split out of this security PR so it stays surgical. They are preserved in the branch history (commit6f0030d) and belong in a separate dependency/CVE-floor PR.